block deserialization with dynafed support#25
block deserialization with dynafed support#25sekulicd wants to merge 15 commits intovulpemventures:masterfrom
Conversation
|
add .idea to gitignore @sekulicd |
|
Please run |
tiero
changed the title
| }; | ||
|
|
||
| const block = new Block(); | ||
| block.version = readUInt32(); |
There was a problem hiding this comment.
I would use the BufferReader from bufferutils.ts to read the bytes instead of read* functions.
You also need to delete the folder and repush with "deleted |
test/block.spec.ts
Outdated
|
|
||
| describe('block deserialization ', () => { | ||
| fixtures.test.forEach(f => { | ||
| block.Block.fromBuffer(Buffer.from(f.hex, 'hex')); |
There was a problem hiding this comment.
this checks if fromBuffer throws an error so LGTM. Best way to test is to check if all the block's fields are deserialized but may be hard to make the fixtures
There was a problem hiding this comment.
I mean we need an it ro specify the test
ie.
| block.Block.fromBuffer(Buffer.from(f.hex, 'hex')); | |
| it('should de-serialize dyna-fed blcoks', () => { | |
| const bock = block.Block.fromBuffer(Buffer.from(f.hex, 'hex')); | |
| //assert block is non empty | |
| }); |
|
I added another test fixture for a block with compact current and null proposed params taken from https://github.com/ElementsProject/rust-elements/blob/acc702c/src/block.rs#L653 and added assertions to check the expected block hash, version, and signBlockWitnessLimit, along with number of transactions. The block hashes resulting from block.getHash() are not matching those expected by the tests in rust-elements. Any ideas why? |
cc/ @sekulicd |
|
thanks @asoltys @sekulicd can you cherry-pick his commit to add the test? asoltys@4a0bb0c |
|
@sekulicd @tiero @louisinger what's the status of this? |
Has been tested by @asoltys and confirmed was working, but we miss to clean it up, solve conflicts etc.. for proper merge in master branch |
5 participants
This adds supports to deserialise dynafed block.
This closes #23
@tiero @asoltys please review.